fix: [AI-696] single-pass env-var resolution for MCP configs by anandgupta42 · Pull Request #697 · AltimateAI/altimate-code

anandgupta42 · 2026-04-11T01:51:10Z

What does this PR do?

Follow-up to #666. Collapses MCP env-var interpolation to a single resolution point at config load time, scoped to env and headers fields only. Fixes two correctness bugs found during consensus code review of #666:

$${VAR} escape broken end-to-end — the two-layer design resolved $${VAR} → ${VAR} at parse time and then re-resolved ${VAR} at launch. There was no syntax left to pass a literal ${SOMETHING} to an MCP server.
Variable-chain injection — with EVIL_VAR="${SECRET}" in the shell (common: Bash PS1, templated snippets, GH Actions leftovers) and a config referencing ${EVIL_VAR}, the two-layer design exposed SECRET's value even though the config only referenced EVIL_VAR. POC confirmed by Gemini during multi-model review.

Changes:

packages/opencode/src/config/paths.ts — exported shared ENV_VAR_PATTERN, resolveEnvVarsInString(), newEnvSubstitutionStats(). Refactored internal substitute() to use the same regex grammar. One implementation, one place to maintain.
packages/opencode/src/mcp/discover.ts — reverted the ConfigPaths.parseText call in readJsonSafe (was substituting across the whole JSON text, including command args and URLs). Added resolveServerEnvVars() helper called from transform(), scoped to env and headers values only. Emits log.warn for unresolved vars with server + source context.
packages/opencode/src/mcp/index.ts — removed resolveEnvVars() and ENV_VAR_PATTERN entirely. The stdio launch site now uses mcp.environment directly — single resolution already happened at config load time.
packages/opencode/test/mcp/env-var-interpolation.test.ts — switched unit tests to ConfigPaths.resolveEnvVarsInString. Added 5 regression tests: single-pass no-chain; $${VAR} end-to-end preservation; chain injection blocked; ${VAR} resolution in remote server headers; ${VAR} in command args / URL is NOT resolved (scope check). Also fixed fragile process.env = {...} teardown.

Review items addressed (from #666 consensus review, #666 (comment)):

Severity	Item	Status
Critical	Double interpolation / `\$\${VAR}` broken / chain injection	✅ Fixed
Major	Regex duplicated between `config/paths.ts` and `mcp/index.ts`	✅ Single source in `config/paths.ts`
Major	Silent empty-string on unresolved `${VAR}`	✅ `log.warn` with server + source + unresolved names
Major	Layer 1 substituted whole JSON text, not just env fields	✅ Scoped to `env` and `headers` only
Major	`catch {}` in `readJsonSafe` swallowed the error	✅ Removed with the block
Minor	`resolveEnvVars` exported at top level of `mcp/index.ts`	✅ Removed
Minor	`mcp.headers` not resolved	✅ Now resolved
NIT	`process.env = {...}` teardown	✅ Uses per-key `delete`

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Issue for this PR

Closes #696

Related: #656, #666

How did you verify your code works?

Unit + integration tests: 36/36 pass in test/mcp/env-var-interpolation.test.ts + test/mcp/discover.test.ts
Full MCP suite: 166/166 pass across test/mcp/*
Marker guard: bun run script/upstream/analyze.ts --markers --base main --strict — clean
TypeScript: zero new errors in touched files (pre-existing TS2589 in config.ts:1322 is unrelated)
Regression tests specifically added:
- $${TEST_MCP_TOKEN} in a discovered .vscode/mcp.json → resolved MCP environment contains literal ${TEST_MCP_TOKEN}
- EVIL_MCP_VAR="${TEST_MCP_SECRET}" in shell, config references ${EVIL_MCP_VAR} → resolved env contains literal ${TEST_MCP_SECRET}, NOT the secret value
- Remote MCP server with Authorization: Bearer ${TEST_MCP_TOKEN} in headers → resolved to Bearer glpat-secret-token
- ${VAR} in command args and URLs → NOT touched (scope guard)

Checklist

I have performed a self-review of my code
My changes follow the existing code style
I have added tests that prove my fix is effective (5 new regression tests)
New and existing unit tests pass locally
I have run the marker guard check locally
I have updated the relevant altimate_change markers

🤖 Generated with Claude Code

Summary by cubic

Collapse MCP env-var interpolation to a single pass at config load time, scoped to env and headers, fixing escape handling and preventing variable-chain injection. Aligns with Linear AI-696 by removing double interpolation and adding clear warnings for unresolved vars.

Bug Fixes
- Preserve $${VAR} as a literal ${VAR} end-to-end.
- Block chain injection when an env var’s value contains ${OTHER}.
- Resolve ${VAR} in remote server headers; do not touch command args or URLs.
- Emit log.warn with server and source when variables are unresolved.
Refactors
- Centralize regex and helpers in ConfigPaths (ENV_VAR_PATTERN, resolveEnvVarsInString, newEnvSubstitutionStats) and reuse in JSON substitution.
- Remove launch-time resolver from mcp/index.ts; use mcp.environment directly since resolution happens at load.
- Simplify readJsonSafe to parse only; resolution now runs in transform() via resolveServerEnvVars() for env/headers fields.

^{Written for commit 9122153. Summary will update on new commits.}

Summary by CodeRabbit

New Features
- Enhanced environment variable substitution in MCP server configurations with improved warning messages listing unresolved variables.
- Added support for tracking unresolved variable names during config loading.
Bug Fixes
- Fixed environment variable chain-injection vulnerability—resolved values are no longer re-interpolated.
- Corrected scope of variable substitution to apply only to env and headers sections, leaving other config fields unchanged.
Tests
- Expanded test coverage for environment variable interpolation and MCP discovery workflows.

Follow-up to #666. The two-layer design (parse-time `ConfigPaths.parseText` in `readJsonSafe` + launch-time `resolveEnvVars` in `mcp/index.ts`) created two correctness bugs: 1. `$${VAR}` escape broken end-to-end: Layer 1 turned `$${VAR}` into literal `${VAR}`; Layer 2 then re-resolved it to the live env value. No syntax remained for passing a literal `${SOMETHING}` to an MCP server. 2. Variable-chain injection: with `EVIL_VAR="${SECRET}"` in the shell and a config referencing `${EVIL_VAR}`, Layer 1 produced the literal `"${SECRET}"` which Layer 2 then resolved to the actual secret — exfiltrating a variable the config never mentioned. The fix collapses to a single resolution point at config load time, scoped to the fields that need it. Changes: - `config/paths.ts`: export shared `ENV_VAR_PATTERN`, `resolveEnvVarsInString()`, and `newEnvSubstitutionStats()`. Internal `substitute()` reuses the same regex grammar — no more duplicate implementation in `mcp/index.ts`. - `mcp/discover.ts`: revert the `ConfigPaths.parseText` call in `readJsonSafe` (was substituting across the whole JSON text, not just env blocks). Add `resolveServerEnvVars()` helper called from `transform()`, scoped to `env` and `headers` values only. Emits `log.warn` for unresolved vars with server and source context. - `mcp/index.ts`: remove `resolveEnvVars()` and `ENV_VAR_PATTERN` entirely. The stdio launch site now uses `mcp.environment` directly — single resolution point already ran at config load time. - `test/mcp/env-var-interpolation.test.ts`: switch to `ConfigPaths.resolveEnvVarsInString`. Add 5 regression tests: single-pass no-chain; `$${VAR}` survives end-to-end; chain injection blocked; `${VAR}` resolved in remote server `headers`; `${VAR}` in command args / URL is NOT resolved (scope check). Also fixes fragile `process.env = {...}` teardown. Addresses the PR #666 consensus code review: - Critical: double interpolation (resolution collapsed to one pass) - Major: regex duplication (single source in `config/paths.ts`) - Major: silent empty-string on unresolved vars (now `log.warn` with context) - Major: whole-JSON-text scope (narrowed to `env` and `headers` only) - Major: swallowed `catch {}` in `readJsonSafe` (removed with the whole block) - Minor: `resolveEnvVars` top-level export (removed) - Minor: `mcp.headers` not resolved (now resolved) Test results: 36/36 tests pass in `test/mcp/env-var-interpolation.test.ts` + `test/mcp/discover.test.ts`. Marker guard clean. No new TS errors in touched files. Relates to #656 and #666.

github-actions · 2026-04-11T01:51:22Z

👋 This PR was automatically closed by our quality checks.

Common reasons:

New GitHub account with limited contribution history
PR description doesn't meet our guidelines
Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

coderabbitai · 2026-04-11T01:51:59Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 61e30239-5c1f-4fc1-866d-40c4e6e3c0b7

📥 Commits

Reviewing files that changed from the base of the PR and between f030bf8 and 9122153.

📒 Files selected for processing (4)

packages/opencode/src/config/paths.ts
packages/opencode/src/mcp/discover.ts
packages/opencode/src/mcp/index.ts
packages/opencode/test/mcp/env-var-interpolation.test.ts

Disabled knowledge base sources:

Jira integration is disabled

You can enable these sources in your CodeRabbit configuration.

📝 Walkthrough

Walkthrough

This PR consolidates environment variable interpolation to a single point at config load time, scoped to env and headers fields only. It extracts shared regex and resolver primitives into ConfigPaths, removes the double-resolution bug from #666 (runtime re-substitution in mcp/index.ts), and applies scoped resolution during MCP config discovery.

Changes

Cohort / File(s)	Summary
Core env-var interpolation exports `packages/opencode/src/config/paths.ts`	Added `ENV_VAR_PATTERN` regex, `EnvSubstitutionStats` interface with `unresolvedNames` tracking, `resolveEnvVarsInString()` for raw-string substitution (no JSON escaping), and `newEnvSubstitutionStats()` initializer. Updated internal `substitute()` to use shared pattern and stats object.
MCP discovery scoping `packages/opencode/src/mcp/discover.ts`	Added `resolveServerEnvVars()` to resolve `${VAR}` / `{env:VAR}` references only within `env` and `headers` blocks with warning emission. Updated `transform()` to accept context and apply scoped resolution. Removed full-text pre-substitution from `readJsonSafe()`.
MCP runtime cleanup `packages/opencode/src/mcp/index.ts`	Removed exported `resolveEnvVars()` function and runtime re-substitution pass. Server transport now forwards `mcp.environment` directly without re-expansion.
Test coverage expansion `packages/opencode/test/mcp/env-var-interpolation.test.ts`	Replaced `resolveEnvVars` unit tests with `resolveEnvVarsInString` tests. Added stats tracking assertions (`dollarUnresolved`, `unresolvedNames`), chain-injection prevention tests, MCP discovery integration coverage, and guard test for interpolation scope (scoped to `env`/`headers` only).

Sequence Diagram(s)

sequenceDiagram
    participant Config as Config Load<br/>(Old Two-Layer)
    participant Parse as Parse Layer<br/>ConfigPaths.parseText
    participant Launch as Launch Layer<br/>resolveEnvVars
    participant MCP as MCP Server
    
    Config->>Parse: readJsonSafe(file)
    Parse->>Parse: Resolve $${VAR} → ${VAR}<br/>Resolve ${VAR} → env value
    Parse->>Launch: pass JSON with resolved values
    Launch->>Launch: Re-resolve ${VAR} patterns<br/>in already-resolved values
    Launch->>MCP: Double-resolved env<br/>(${EVIL_VAR} chain injection)
    
    Note over Parse,Launch: Bug: No escape syntax,<br/>Chain injection possible

sequenceDiagram
    participant Load as Config Load<br/>(New Single-Layer)
    participant Discover as Discover MCP<br/>discover.ts
    participant Scope as Scoped Resolver<br/>resolveServerEnvVars
    participant Transport as Transport<br/>mcp/index.ts
    participant MCP as MCP Server
    
    Load->>Discover: readJsonSafe(file, no pre-sub)
    Discover->>Discover: parseJsonc() raw config
    Discover->>Scope: transform() with context<br/>{server, source}
    Scope->>Scope: Resolve vars in env/<br/>headers only
    Scope->>Scope: Emit warning with<br/>unresolved names
    Scope->>Discover: Scoped values<br/>${ESCAPED_VAR} preserved
    Discover->>Transport: Pass resolved env
    Transport->>MCP: Forward env directly<br/>No re-expansion
    
    Note over Scope: Single pass,<br/>scoped, no chain injection

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

#666: Direct predecessor that introduced the double-resolution regression—this PR fixes its two-layer approach by collapsing to single-pass, scoped resolution.
#655: Modifies the same env-var interpolation primitives in packages/opencode/src/config/paths.ts, standardizing the shared ENV_VAR_PATTERN and resolver behavior.

Suggested labels

needs:compliance

Poem

🐰 Two layers of magic caused quite a fright,
Variables doubled—oh what a sight!
Now one golden pass at config load time,
Scoped to just env and headers—sublime!
No chains, no escapes that slip through the cracks,
The rabbit hops forward; regression rolls back! 🥕

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/mcp-env-var-interpolation-followup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-04-11T01:51:59Z

👋 This PR was automatically closed by our quality checks.

Common reasons:

New GitHub account with limited contribution history
PR description doesn't meet our guidelines
Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

github-actions · 2026-04-11T01:52:52Z

👋 This PR was automatically closed by our quality checks.

Common reasons:

New GitHub account with limited contribution history
PR description doesn't meet our guidelines
Contribution appears to be AI-generated without meaningful review

If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you.

github-actions bot added needs-review:blocked contributor labels Apr 11, 2026

github-actions bot closed this Apr 11, 2026

claude bot reviewed Apr 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [AI-696] single-pass env-var resolution for MCP configs#697

fix: [AI-696] single-pass env-var resolution for MCP configs#697
anandgupta42 wants to merge 1 commit intomainfrom
fix/mcp-env-var-interpolation-followup

anandgupta42 commented Apr 11, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

claude bot left a comment

Uh oh!

coderabbitai bot commented Apr 11, 2026 •

edited

Loading

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

anandgupta42 commented Apr 11, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

Issue for this PR

How did you verify your code works?

Checklist

Summary by cubic

Summary by CodeRabbit

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

coderabbitai bot commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

Poem

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

github-actions bot commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

anandgupta42 commented Apr 11, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Apr 11, 2026 •

edited

Loading